fix(javascript): fix types not being recognized for NodeNext module resolution #4540
Conversation
…esolution Signed-off-by: Gokhan Kurt <[email protected]>
|
Interesting, I'd never heard of |
Is the test suite for that absolutely necessary? Imho, we find a working configuration, test it manually in different scenarios, iterate if necessary. This isn't something that will change much. Most npm projects work with this approach. Also, these changes should not affect runtime at all. This is only a Typescript change, and if needed we can test it with a simple setup of running Btw, sharing the results from AreTheTypesWrong: Before: After: Here are the problems:
|
|
The lack of automated testing is the reason we are where we are. Every now and then, someone comes with a solution to a problem specific to their usage, whilst in parallel the ecosystem keeps evolving. We implement that change and that creates a problem for another use case. |
|
@KurtGokhan interesting tool you used for this information here. I didn't know that before. As a test I let it check the new ANTLR4 TypeScript target antlr4ng and it came out as:
Maybe you want to try that out? I'm open to accept a PR to fix the last remaining issue. |
|
@mike-lischke "Masquerading as ESM" shouldn't cause any problems as long as you are not using default exports. It may not need a fix. I will try out your implementation too, thanks. |
runtime/JavaScript/package.json
Outdated
| "node": { | ||
| "types": "./src/antlr4/index.d.ts", | ||
| "types": "./src/antlr4/index.d.cts", |
There was a problem hiding this comment.
I believe this should be removed ?
runtime/JavaScript/package.json
Outdated
| "import": "./dist/antlr4.node.mjs", | ||
| "require": "./dist/antlr4.node.cjs", | ||
| "default": "./dist/antlr4.node.mjs" | ||
| }, | ||
| "browser": { | ||
| "types": "./src/antlr4/index.d.ts", | ||
| "types": "./src/antlr4/index.d.cts", |
There was a problem hiding this comment.
I believe this should be removed ?
There was a problem hiding this comment.
Yes, removed it and tested with different module resolutions like NodeNext, Bundler and Node. Seems working.
|
@parrt let's see if we can get that one in |
…resolution ; Conflicts: ; runtime/JavaScript/package.json
|
@Partt blessed |
|
Looks like there are conflicts. |
|
@parrt I see conflicts if you rebase, not if you merge ? |
Fixes #4218
The problem was, since the
package.jsonhastype: module, Typescript is treating regulard.tsfiles as module. But thed.tsfiles are generated manually and they don't comply with how a module file should look like (e.g. missing file extensions on imports). So they are essentially more like CommonJS files.Luckily, there is a way to override the module detection. If the file has
ctsorcjsextension, Typescript will treat that file as CommonJS. In this case, addingctsextension only to theindexfile was enough.Ideally, the TS files should be built with
tscin the future to ensure the files comply with the corresponding module format.Also added a
filesfield to the package.json so that only relevant files are included in the NPM bundle. Previously it also included files likespec,src, and dev config files, which made it confusing to debug.Tested my changes locally with various
moduleResolutionoptions likeNode, Node10, Node16, NodeNext, Bundlerand they work flawlessly. Only theClassicoption doesn't work but it also doesn't work on upstream and it doesn't work for most other packages as well.Alternative solutions
All of the other solutions require changes in more files but I will leave them here as an option.
The first obvious solution is to add extensions to all the TS files.
I checked various other packages like
lodash-esand checked why they don't have this problem even though they don't have extension in TS files. Turns out, they don't havetype: modulein their package.json, so files are treated as CommonJS by default, even though the packages are ES packages.So one of the solutions is to remove
type: moduleand convert alljsfiles tomjs.